New profile UI #17521

nguyentruongky · 2023-10-04T13:02:07Z

Summary

Implement profile UI (Issue Implement profile view #17393 )

Review notes

I can't achieve scrolling animation yet.
Missing Jump to floating button.
Some TODO tags for missing screens

Functional

user profile

Before and after screenshots comparison

Figma	iOS
https://www.figma.com/file/PiN0uwRgZj4L9U4N7xO7Xb/Profile-for-Mobile?node-id=3773%3A221861&mode=dev	Screenshot

status: ready

ghost · 2023-10-04T13:02:12Z

Hey @nguyentruongky, and thank you so much for making your first pull request in status-mobile! ❤️ Please help us make your experience better by filling out this brief questionnaire https://goo.gl/forms/uWqNcVpVz7OIopXg2

J-Son89 · 2023-10-04T20:00:58Z

src/quo2/foundations/colors.cljs

@@ -121,6 +121,11 @@
 (def white-opa-90 (alpha white 0.9))
 (def white-opa-95 (alpha white 0.95))

+(def magenta "#EC266C")


you can remove these colors, we already have them defined. They are also a more dynamic set of colors based on the users customization-color (set in the onboarding phase)

instead when you go to use this customization-color you should pass as

(:require [src/quo2/foundations/colors.cljs :as colors]) ... (let [{:keys [customization-color]} (rf/sub [:profile/multiaccount])] [quo/button { :customization-color (colors/custom-color customization-color 50 40) }

that's what it will be for the use case you are using it for anyway where 40 is 40% opacity.
50 there is a suffix that is used in the mapping, you can see it if you trace through the custom-color function etc

if you search in the codebase you'll see many other examples of customization-color

Thanks @J-Son89 it's changed.

J-Son89 · 2023-10-04T20:02:08Z

src/status_im2/contexts/profile/profiles/views_v2.cljs

@@ -0,0 +1,123 @@
+(ns status-im2.contexts.profile.profiles.views-v2


I think this namespace is new anyway so there's no need for v2 👍

Changed. It was in status_im. I just figured out it needs to be in status_im2 at last minutes.

J-Son89 · 2023-10-04T20:02:42Z

src/status_im2/contexts/profile/profiles/views_v2.cljs

@@ -0,0 +1,123 @@
+(ns status-im2.contexts.profile.profiles.views-v2
+  (:require [clojure.string :as string]
+            [quo.core :as quo]


please don't use any components or code that lives in quo. namespaces.
We strictly want to use quo2 code 👍

I think that might also be why you needed to adjust so many icon files as you are working from mostly old code :/

Changed to quo2 most of things I can. Please review again.

Hi @nguyentruongky, the changes you made look great. I would prefer to discuss with you before reviewing again as there are some ways that I should be able to help you find the correct code/components in the codebase. let's chat on discord 👍

Thanks @J-Son89 How can I reach out to you on discord. Just registered with username adventurous_beagle_60958

J-Son89 · 2023-10-04T20:06:33Z

src/status_im2/contexts/profile/profiles/views_v2.cljs

+            [quo.design-system.colors :as colors]
+            [re-frame.core :as re-frame]
+            [reagent.core :as reagent]
+            [status-im.ui.components.copyable-text :as copyable-text]


same for the status-im code, this is all old and needs to be removed on our side. Yet for this pr you should also not need it. This code should exist elsewhere, it's quite tricky to see what you are using here so please reach out for individual questions.

in the case of [status-im.ui.components.react :as react]
it's preffered to use [react-native.core :as rn]. you'll see a lot rn/view etc in the codebase 👍

Copied from the original file and changed some as current implementation, so some required I still don't understand clearly. Deleted most of them.

sure, np. if there is some expectation or something that is unclear it's best to discuss first. I suggest reaching out over discord as it will be faster to discuss there. 👍

J-Son89 · 2023-10-04T20:07:36Z

src/status_im2/contexts/profile/profiles/views_v2.cljs

+            [status-im2.common.qr-code-viewer.view :as qr-code-viewer]
+            [status-im2.config :as config]
+            [utils.i18n :as i18n])
+  (:require-macros [status-im.utils.views :as views]))


why do you need this macro? 🤔 what did you use it to achieve? curious but I would assume it's not necessary

Sorry, copied it from the original file.

J-Son89 · 2023-10-04T20:08:14Z

src/status_im2/contexts/profile/profiles/views_v2.cljs

+
+(views/defview share-chat-key
+  []
+  (views/letsubs [{:keys [address ens-name]} [:popover/popover]


hmm @flexsurfer is there better sub for @nguyentruongky to get this address & ens-name? 🤔

J-Son89 · 2023-10-04T20:10:46Z

some screenshot or a video recording would be greatly appreciated! :)

J-Son89 · 2023-10-04T20:11:34Z

src/quo/components/list/item.cljs


 (defn size->container-size
  [size]
  (case size
    :small 52
-    64))
+    (min size 64)))


this seems like it's wrong to me, why did you make these changes? 🤔

Changed. Needed to changed to smaller size to match the design.

J-Son89 · 2023-10-04T20:14:24Z

src/quo/components/list/item.cljs

@@ -70,29 +70,31 @@
        children))

 (defn icon-column
-  [{:keys [icon icon-bg-color icon-color size icon-container-style]}]
+  [{:keys [icon icon-bg-color icon-color icon-size icon-container-style]}]


hmm, imo this prop name doesn't need to be changed or is there a particular reason for it?

J-Son89 · 2023-10-04T20:16:00Z

src/quo/core.cljs

@@ -16,6 +16,7 @@

 (def text text/text)
 (def header header/header)
+(def header-action header/header-action)


see other comments about using quo2 components only. Also we generally try to avoid exporting multiple components from the one file, so in general I would prefer they were separated to their own view file.
However in this case it's not necessary as I think we should avoid using this code completely 👍

Removed it.

J-Son89 · 2023-10-04T20:17:45Z

src/status_im/ui/screens/profile/user/styles.cljs

+(def radius 16)
+
+(def top-background-view
+  {:background-color colors/magenta-opa-40


this is where you should be using the custom-color approach
so it's better here to do:

(defn top-background-view [customization-color] {:background-color (colors/custom-color customization-color 50 40)

Thanks. Changed all its appearance.

J-Son89 · 2023-10-04T20:18:47Z

src/status_im/ui/screens/profile/user/styles.cljs

+   :top              0
+   :left             0
+   :height           400
+   :width            400


any particular reason for 400? would:bottom 0 :right 0 as well not be better? 🤔

Just want to keep the view as the solid background (magenta) on the top when pull down.
if use bottom 0, the magenta background is visible when scroll to the bottom.

J-Son89 · 2023-10-04T20:19:38Z

src/status_im/ui/screens/profile/user/styles.cljs

+
+(def toolbar
+  {:padding-bottom  16
+   :padding-top     (safe-area/get-top)


@smohamedjavid this is alright to do, or we normally get this value at the rendering of the component view in a let var? 🤔

You are right. We usually get the safe area value from the component's let var. 👍

;; view.cljs (let [top (safe-area/get-top)] [rn/view {:style (style/toolbar top)}]) ;; style.cljs (defn toolbar [top] {:padding-bottom 16 :padding-top top :flex-direction :row :align-items :center :justify-content :space-between})

J-Son89 · 2023-10-04T20:20:35Z

src/status_im/ui/screens/profile/user/styles.cljs

+   :margin-bottom     64
+   :overflow          "hidden"
+   :border-radius     radius
+   :background-color  colors/danger-50-opa-20})


imo for danger it's better to also use the colors mechanism-> i.e in this case.
(colors/custom-color :danger 50 20)

Thanks. Changed.

J-Son89 · 2023-10-04T20:21:55Z

src/status_im/ui/screens/screens.cljs

@@ -51,7 +51,7 @@
    [status-im.ui.screens.profile.contact.views :as contact]
    [status-im.ui.screens.profile.group-chat.views :as profile.group-chat]
    [status-im.ui.screens.profile.seed.views :as profile.seed]
-    [status-im.ui.screens.profile.user.views :as profile.user]
+    [status-im2.contexts.profile.profiles.views-v2 :as profile.user]


it would be preferable if you register the new screen in status-im2.navigation.screens namespace instead 👍

J-Son89 · 2023-10-04T20:22:30Z

src/status_im/ui/screens/screens.cljs

@@ -74,7 +74,12 @@
    [status-im.ui.screens.wallet.settings.views :as wallet-settings]
    [status-im.ui.screens.wallet.swap.views :as wallet.swap]
    [status-im.ui.screens.wallet.transactions.views :as wallet-transactions]
-    [status-im2.contexts.chat.group-details.view :as group-details]))
+    [status-im2.contexts.chat.group-details.view :as group-details]
+    [status-im2.contexts.chat.home.view :as chat-home]


it's unclear why you have added all of these screens here? we already have them registered in the app? any particular reason? 🤨

Want to navigate to the chat home screen. Just removed.

nguyentruongky · 2023-10-05T08:40:19Z

@J-Son89 added a video at https://monosnap.com/file/M7TUAbEHn8eSRCJKzWD3zW41NY7t83.

flexsurfer

could you pls rollback all icon files

flexsurfer · 2023-10-05T09:55:49Z

src/status_im2/contexts/profile/profiles/views.cljs

+
+      [rn/view {:flex 1 :style styles/container-style}
+
+       components/top-background-view


this looks strange, could you elaborate?

flexsurfer · 2023-10-05T09:56:10Z

src/status_im2/contexts/profile/profiles/views.cljs

+                  ens-name address
+                  key-uid]
+           :as   account}
+          @(re-frame/subscribe [:profile/multiaccount])


please use rf/sub instead of @(re-frame/subscribe

flexsurfer · 2023-10-05T09:56:33Z

src/status_im2/contexts/profile/profiles/views.cljs

+          has-picture @(re-frame/subscribe [:profile/has-picture])
+          link (universal-links/generate-link :user :external (or ens-name address))]
+
+      [rn/view {:flex 1 :style styles/container-style}


this looks strange, why flex outside style map?

flexsurfer · 2023-10-05T09:57:28Z

src/status_im2/navigation/screens.cljs

@@ -129,6 +130,12 @@
     :on-focus  [:onboarding/overlay-dismiss]
     :component profiles/view}

+    {:name      :my-profile
+     :options   {:theme  :dark
+                 :layout options/onboarding-layout}


could you elaborate on why /onboarding-layout is used?

flexsurfer · 2023-10-05T09:57:43Z

ios/Podfile.lock

@@ -772,7 +772,7 @@ SPEC CHECKSUMS:
  RNLanguages: 962e562af0d34ab1958d89bcfdb64fafc37c513e
  RNPermissions: ad71dd4f767ec254f2cd57592fbee02afee75467
  RNReactNativeHapticFeedback: 2566b468cc8d0e7bb2f84b23adc0f4614594d071
-  RNReanimated: 43adb0307a62c1ce9694f36f124ca3b51a15272a


please rollback this file

flexsurfer · 2023-10-05T09:57:58Z

src/quo/components/header.cljs

@@ -80,7 +80,7 @@
   :flex            1})

 (defn header-action
-  [{:keys [icon label on-press disabled accessibility-label]}]


please rollback this file

flexsurfer · 2023-10-05T09:58:02Z

src/quo/components/list/item.cljs

@@ -90,9 +90,11 @@
 (defn title-column
  [{:keys [title text-color subtitle subtitle-max-lines subtitle-secondary
           title-accessibility-label size text-size title-text-weight
+           title-style


please rollback this file

flexsurfer · 2023-10-05T09:58:15Z

src/status_im/ui/screens/screens.cljs

@@ -530,6 +526,11 @@
    :options   {:insets {:bottom? true
                         :top?    true}}
    :component bookmarks/new-bookmark}
+   {:name      :browser


please rollback this file

flexsurfer · 2023-10-05T09:59:32Z

src/status_im2/contexts/profile/profiles/views.cljs

+         :on-switch-profile nil
+         :on-show-qr        on-share}]
+
+       [rn/scroll-view


better to use flat-list

flexsurfer · 2023-11-08T11:08:56Z

stale

nguyentruongky added 5 commits October 4, 2023 10:55

add new icons + resources

468c05e

implement profile v2

3b5397d

customize the spacing to match the design

7e50f4f

move new profile to status_im2

62751cb

Merge develop

d66bd94

flexsurfer added external external-c labels Oct 4, 2023

J-Son89 requested review from J-Son89, briansztamfater, jo-mut and siddarthkay October 4, 2023 19:53

J-Son89 reviewed Oct 4, 2023

View reviewed changes

Changes as reviewed

da460d0

Merge branch 'develop' into new-profile-ui

0da41c7

flexsurfer requested changes Oct 5, 2023

View reviewed changes

flexsurfer closed this Nov 8, 2023

		@@ -0,0 +1,123 @@
		(ns status-im2.contexts.profile.profiles.views-v2


		[rn/view {:flex 1 :style styles/container-style}

		components/top-background-view

New profile UI #17521

New profile UI #17521

Conversation

nguyentruongky commented Oct 4, 2023 • edited Loading

Summary

Review notes

Functional

Before and after screenshots comparison

ghost commented Oct 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nguyentruongky Oct 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

J-Son89 commented Oct 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nguyentruongky commented Oct 5, 2023

flexsurfer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flexsurfer commented Nov 8, 2023

nguyentruongky commented Oct 4, 2023 •

edited

Loading

nguyentruongky Oct 5, 2023 •

edited

Loading